Skip to content

revert sds name addition to gateway#781

Merged
rshriram merged 1 commit intoistio:release-1.1from
rshriram:revert_sds
Jan 31, 2019
Merged

revert sds name addition to gateway#781
rshriram merged 1 commit intoistio:release-1.1from
rshriram:revert_sds

Conversation

@rshriram
Copy link
Copy Markdown
Member

While we continue to discuss the right way of introducing SDS config
into the gateway, revert the existing one which is not the right
way of introducing the feature as it goes against our current
API design, forcing users to specify all options (files, sds names, etc.)
without any idea of which one would be picked by the underlying system.
This might be useful for automation code but definitely not for end users
authoring configs.

Signed-off-by: Shriram Rajagopalan shriramr@vmware.com

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
@googlebot googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Jan 30, 2019
@istio-testing istio-testing requested a review from hklai January 30, 2019 20:50
@rshriram rshriram requested review from louiscryan and wenchenglu and removed request for andraxylia and hklai January 30, 2019 20:53
@wenchenglu wenchenglu requested a review from JimmyCYJ January 30, 2019 22:35
@wenchenglu
Copy link
Copy Markdown

@costinm
@myidpt

is this the same issue you guys have discussed recently? have we reached agreement?

Copy link
Copy Markdown
Contributor

@costinm costinm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

Let's try to use the gateway name in the implementation, so we can solve the immediate problem in ingress for 1.1 - we can add more APIs and customization in 1.2 after user feedback,

@istio-testing
Copy link
Copy Markdown
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: costinm, rshriram

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@myidpt
Copy link
Copy Markdown

myidpt commented Jan 30, 2019

@rshriram
Do you agree on Costin's comment:
"Let's try to use the gateway name in the implementation, so we can solve the immediate problem in ingress for 1.1 - we can add more APIs and customization in 1.2 after user feedback"
This means in 1.1, when the sds_name is not specified, we will use the "portname.gatewayname.namespace" to infer the the secret.

We need to make an agreement for 1.1 so that we can make corresponding changes on Pilot early. Otherwise, we will have risk on delivering the feature in 1.1.

@myidpt
Copy link
Copy Markdown

myidpt commented Jan 30, 2019

@wenchenglu Please wait until Shriram agrees on the plan here:
#781 (comment)
My concern is if we don't have consensus on 1.1, we will have trouble moving forward to deliver the feature in 1.1.

@rshriram
Copy link
Copy Markdown
Member Author

I would love to use the portName.gatwayName.namespace .. But how do you infer the name of the secret from this? You would have to read the gateway spec from k8s, and extract the specific gateway from the specific namespace, get to the specific server in the gateway using the portName. And from there, you look at the cert paths in the server (/etc/istio/certs//..), extract the secret and then fetch the secret.

If this is okay by you, lets go for it. There is no need for any additional field.

@rshriram rshriram merged commit d5da499 into istio:release-1.1 Jan 31, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants